see below

Clean, well-executed security hardening. Validation logic is correct and thoroughly tested. Approving with one suggestion inline.

Strengths:

• O(1) length check gates all downstream work — good defense against oversized input on an edge server
• Lowercase hex enforcement prevents intermediary case-normalization from creating fake-valid IDs
• debug_assert! in the generator ensures validator and generator never drift apart
• Logging hygiene — invalid values logged with len={} only, never the raw attacker-controlled string
• Excellent test coverage with 8+ new/updated test cases
• Shared VALID_SYNTHETIC_ID constant eliminates test fragility across 3 modules
• CHANGELOG and docs properly updated

Note: The lol_html bump (e295f3a) in Cargo.toml/Cargo.lock appears to be a merge artifact from main — unrelated to the synthetic ID validation. Not a problem, just flagging it.

Summary

Well-executed security hardening PR that validates synthetic ID format on inbound header and cookie values. The validation logic is correct and test coverage is thorough. One gap in the revocation path needs attention.

Blocking

🔧 wrench

• Revocation path bypasses validation: existing_ssc_cookie in publisher.rs:379-387 is read from the raw cookie jar before get_or_generate_synthetic_id runs validation. This raw, unvalidated value is then: (1) logged at log::info! level (line 381) — potentially logging attacker-controlled content, and (2) passed to delete_consent_from_kv as a KV store key (line 387) — allowing a crafted cookie value to target arbitrary KV keys for deletion. This is the same class of injection this PR aims to fix. Suggested fix: make is_valid_synthetic_id pub(crate) and validate existing_ssc_cookie before use, or restructure so the already-validated synthetic ID is reused for revocation. At minimum, track as a follow-up issue.

Non-blocking

🏕 camp site

• Clarify old vs new validation layers (cookies.rs:38): The old synthetic_id_has_only_allowed_chars (permits [a-zA-Z0-9._-]) remains for outbound cookie sanitization, while the new is_valid_synthetic_id in synthetic.rs is strictly tighter for inbound validation. A brief comment noting this layered relationship would help future readers.

⛏ nitpick

• PR body references stale crates/common/src/ paths: actual paths are crates/trusted-server-core/src/...
• PR checklist says "tracing macros": project uses log crate, not tracing

CI Status

• fmt: PASS
• clippy: PASS
• rust tests: PASS
• js tests: PASS

Summary

Clean, well-designed security fix that validates inbound synthetic IDs from the x-synthetic-id header and synthetic_id cookie. The is_valid_synthetic_id() validator is correct, defense-in-depth layering across synthetic.rs, cookies.rs, and publisher.rs is sound, and test coverage is thorough.

Non-blocking

📌 out of scope

• Synthetic ID still logged at INFO in proxy.rs redirect handler: The redirect handler at proxy.rs:717 still logs the validated synthetic ID value at log::info!. Low risk since the value is already validated, but inconsistent with the PR's stated goal of demoting ID values to DEBUG. Worth a follow-up.

🤔 thinking

• split_once('.') vs old split('.') is subtly correct: The new approach naturally handles multi-dot values because the total length constraint (71 = 64 + 1 + 6) means extra dots fail the alphanumeric suffix check. The dot_in_suffix test covers this. Sound design choice.

👍 praise

• Excellent defense-in-depth in publisher.rs revocation path (publisher.rs:382-399): Always expires the cookie regardless of validity, only passes validated IDs to delete_consent_from_kv, logs only the length of invalid values. Prevents malicious cookie values being used as KV keys.
• Synthetic ID values removed from log output (synthetic.rs): Old code logged actual ID values at trace level; new code logs only structural information. Eliminates log-exfiltration of pseudonymous identifiers.
• O(1) length guard before character scanning (synthetic.rs:42): Constant-time length check before iteration is a nice DoS mitigation for oversized inputs.
• Comprehensive test coverage: Covers invalid header/cookie rejection, header precedence, fallthrough behavior, generation on invalid input, and edge cases (uppercase hex, oversized input, empty string, dot in suffix, extra segments, missing suffix, non-hex, non-alphanumeric suffix).

⛏ nitpick

• CHANGELOG says "64-hex-hmac" but validator requires lowercase hex (CHANGELOG.md:12): Saying "64-lowercase-hex-hmac" would be more precise. Minor since the code is authoritative.

CI Status

• fmt: PASS
• clippy: PASS
• rust tests: PASS
• js tests: PASS